Review: SEARCH slow-start implementation (draft-chung-ccwg-search-09)

Clean, well-structured implementation. The algorithm logic — bin-based delivery-rate tracking, interpolated lookback, and normalized-difference exit criterion — reads correctly against the draft. The SlowStart trait extension is minimal and the integration into ClassicCongestionController is straightforward.

Key observations:

• Floating-point elimination: The earlier feedback from @larseggert about avoiding f64 has been addressed — all arithmetic now uses integer fixed-point with SCALE = 100. Nice.

• Division-by-zero risk: The initialize guard only rejects Duration::ZERO RTTs, but the computed bin_duration can still be zero for very small non-zero RTTs (see inline comment). This creates panic paths in update_bins and on_packets_acked.

• Drain-phase omission: The decision to skip the draft §3.2 drain-phase is well-reasoned given CUBIC's buffer-filling behavior. The TODO for telemetry capture of the drain-target is a good follow-up.

• Tests: The PR description notes tests will be added separately. The SEARCH algorithm has several interesting edge cases worth covering: initialization on first ACK, bin skipping/reset for stale data, the prev_idx <= W early-return guard, compute_sent interpolation boundaries, and the exit threshold itself. Looking forward to seeing those.

• Spec divergences are well-documented: The NOTE comments for divergences from draft-09 (reset mechanism, bit-shifting optimization, interpolation direction fix, drain-phase) are clear and cite the relevant draft sections — this is exactly the right approach for an implementation tracking an evolving draft.

No blocking issues found. The bin_duration == 0 panic path (inline) is the most important item to address.

Pull request overview

Adds the SEARCH (Slow start Exit At Right CHokepoint) slow-start exit algorithm to neqo-transport congestion control, enabling heuristic slow-start exit based on delivery-rate flattening per the SEARCH draft.

Changes:

• Introduces cc::Search and wires it into PacketSender for NewReno and Cubic.
• Extends the SlowStart trait to accept per-ACK delivered bytes and now, and per-send packet bytes; updates HyStart/ClassicSlowStart and related tests accordingly.
• Updates clippy doc identifier allowlist for “CHokepoint”.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Clean implementation of SEARCH draft-09. The algorithm logic — bin-based delivery-rate tracking, interpolated lookback via compute_sent, and normalized-difference exit criterion — reads correctly against the spec. Constants (W=10, THRESH=0.26, WINDOW_SIZE_FACTOR=3.50) all match.

The intentional divergences are well-documented with rationale and spec citations:

• Stale-data reset (§ missed bins): good addition for app-limited and flow-control-limited scenarios.
• Drain-phase omission (§3.2): well-reasoned given CUBIC's buffer-filling behavior; the TODO for telemetry capture is a good follow-up.
• Interpolation direction fix: correcting the draft's forward interpolation to backward — noted as discussed with the SEARCH team.

The SlowStart trait extension is minimal. Making on_packet_sent a default no-op is a good design choice — it keeps ClassicSlowStart simple while letting SEARCH and HyStart override only what they need. The new new_acked_bytes and now parameters are well-motivated by SEARCH's delivery-rate tracking needs.

Two inline findings (neither blocking): a clippy::cast_possible_truncation lint issue with as usize casts, and a minor 32-bit arithmetic overflow concern in compute_sent.

The previous bot-review items (typos, test parameter confusion) appear to have been addressed in the latest commit.

Good implementation of SEARCH draft-09. The integer-math refactor from floating point is clean, the circular buffer indexing is correct (the prev_idx <= W guard ensures compute_sent never underflows on old - 1), and the spec divergences (no drain phase, optional missed-bin reset) are well-documented with rationale.

Architecture: The SlowStart trait extension (new_acked_bytes, now) is a reasonable approach — the new parameters are plumbed through cleanly and existing impls (ClassicSlowStart, HyStart) just ignore them. The on_packet_sent default body {} is a nice simplification vs. requiring every impl to provide a no-op.

Key items from this review:

• 32-bit overflow in norm_diff computation (same class as the interpolation issue already discussed, but different location)
• u128 as usize casts should use usize::try_from for consistency with update_bins
• debug_assert!(false) gives no visibility in release builds — consider adding a qwarn!

Tests are still pending per the PR description — looking forward to seeing those cover the exit condition, missed-bin reset, and edge cases (zero/tiny RTT, bins_last_rtt = 0).

Summary

Clean implementation of SEARCH 4.0 (draft-09) that integrates well via the existing SlowStart trait. The trait extension is nicely done — adding a default no-op for on_packet_sent avoids boilerplate in implementors that don't need it. Divergences from the spec (no drain phase, interpolation direction fix, optional reset on missed bins) are well-documented with rationale and spec links.

The circular buffer arithmetic is correct — I verified that NUM_ACKED_BINS (11) and NUM_SENT_BINS (26) are exactly sized to avoid aliasing given the access patterns in compute_delv and compute_sent, including the EXTRA_BINS guard at line 266.

The new tests cover initialization, bin advancement, skipped-bin propagation, reset/reinit, and the end-to-end exit condition. A few targeted edge-case tests would round out coverage (noted inline).

Most arithmetic concerns (32-bit overflow, bare as usize casts) were raised in prior review rounds — I've added concrete suggestions for the remaining instances. The debug_assert! + #[should_panic] interaction is a new finding that should be addressed before merge.

Summary

The SEARCH implementation is solid. The algorithm is faithfully mapped from draft-chung-ccwg-search-09, divergences are clearly documented with reasoning (drain-phase skip, reset on stale data, backward interpolation fix), and the integer arithmetic is careful — u64 widening for 32-bit safety, saturating_* ops, try_from with proper fallbacks. The SlowStart trait extension is clean: new parameters have defaults, existing implementors adapt mechanically.

Test coverage for the core algorithm is good: initialization, bin advancement, skipped-bin propagation, reset, warming-up boundary, RTT inflation guard, steady delivery rate, and exit detection are all exercised. A few bot-suggested additions remain open (e.g., prev_sent == 0 path, asserting is_none() during warm-up iterations) and would further strengthen the suite.

Orthogonal changes

The PR bundles several changes unrelated to SEARCH that make review and bisection harder:

• send_stream.rs: Reverts the effective_priority caching from the just-merged #3592, replacing it with on-the-fly computation. The two tests verifying retransmission priority routing are deleted without replacement — see inline comment.
• pace.rs: Migrates pacer arithmetic from u64 to u128 to avoid overflow on high-BDP paths. Correct and worthwhile, but an independent improvement.
• benches/common.rs: Refactors the benchmark harness and removes the flow-controlled benchmark configuration entirely — see inline comment.

Consider splitting these into their own PRs.

Open items from other reviewers

• @larseggert requested changing the slow-start-exit log from qinfo! back to qdebug! — no response yet.
• @larseggert suggested guarding the #[should_panic] test with #[cfg(debug_assertions)] or expecting the assertion text — no response yet.

Review Summary

Clean implementation of SEARCH draft-09 as a SlowStart trait implementation. The bin-based delivery-rate tracking, integer-only arithmetic, and deliberate divergences from the draft (no drain phase, optional reset on stale bins) are well-reasoned and clearly documented.

What looks good

• Correctness: The core algorithm — cumulative byte counters snapshotted into time-binned circular buffers, with linear interpolation for sub-bin lookback — is faithful to the draft. The u64 widening for intermediate computations addresses the 32-bit overflow concerns raised earlier.
• Guard chain: The WarmingUp → RttInflated → ZeroSent → threshold check cascade in evaluate() correctly handles all degenerate states before reaching the hot path.
• Test coverage: Good coverage of initialization, bin advancement, skipped-bin propagation, reset/re-init, steady-rate continuation, and exit detection. The boundary tests (warming_up, inflated_rtt_is_guarded) exercise the exact guard thresholds.
• Trait changes: The SlowStart trait extensions (sent_bytes, new_acked_bytes, now) are backward-compatible via default impls, and all existing implementors are updated.

Items to consider

• Spurious recovery interaction: See inline comment — on_spurious_congestion_event can restore slow start without calling slow_start.reset(), leaving SEARCH with stale bin data. Low risk but worth documenting or addressing.
• EXTRA_BINS invariant: See inline comment — reinforcing the earlier suggestion for a compile-time assertion to guard the relationship between buffer sizes.
• compute_sent precondition: See inline comment — debug_assert!(old >= 1 && new >= 1) would make the underflow precondition explicit.
• First RTT quality (existing thread): The bin_duration is fixed from the first ACK's RTT, which tends to be inflated. The min(first, second) suggestion from @mxinden seems reasonable to raise with the draft authors, though the current behavior is spec-compliant.

No blocking issues found. The implementation is solid and the omission of the drain phase is a pragmatic choice well-suited to the Cubic CA behavior.

Clean implementation of SEARCH 4.0 with well-documented spec divergences (no drain phase, mandatory reset on stale bins, no bit-shifting optimization). The SlowStart trait extension is backward-compatible — new parameters get default no-op implementations where appropriate, and existing implementors (ClassicSlowStart, HyStart) are updated mechanically.

Algorithm correctness: The circular buffer indices never alias thanks to tight-but-valid sizing (NUM_SENT_BINS = W + EXTRA_BINS + 1 = 26, max lookback = 25). The 32-bit overflow fix using u64 widening in compute_sent/compute_delv is sound. The integer math at SCALE = 100 preserves sufficient precision for the threshold comparison, and the two-step division in initialize (/ SCALE / W) is equivalent to a single / (SCALE * W) since Duration tracks nanoseconds internally.

Test coverage: Good coverage of core algorithm paths — warmup boundary, steady-state continuation, delivery-rate-driven exit, RTT inflation guard, zero-sent guard, reset/reinit cycle, and the bin_duration == 0 edge case. One gap noted in my inline comment: no integration test exercises SEARCH through the full ClassicCongestionController pipeline.

One architectural observation: SEARCH's cumulative byte counters (acked_bytes / sent_bytes) have a subtle interaction with the is_app_limited early return in ClassicCongestionController::on_packets_acked. During app-limited periods in slow start, on_packet_sent still increments sent_bytes (gated only by in_slow_start()), but on_packets_acked is skipped entirely — so acked_bytes falls behind. This is unlikely to matter in practice (app-limited during slow start is rare, and the update_bins reset mechanism guards against large gaps), but it's worth being aware of as the algorithm matures.

Solid implementation of SEARCH draft-09. The integer-only arithmetic, circular buffer design with separate acked_bins/sent_bins sizing, and the EXTRA_BINS headroom for RTT inflation are well thought out. The widening to u64 for intermediate computations avoids 32-bit overflow in the critical path. Spec divergences (no drain phase, optional reset on stale bins) are clearly documented with rationale — good practice for a draft that's still evolving.

Key architectural observations:

• Omitting the drain phase is a pragmatic call given CUBIC's buffer-filling behavior. Capturing the drain target in telemetry (as noted in the TODO) will be important for validating this decision.
• record_acked_bytes placement before the app-limited early return is correct for its stated purpose. The counter asymmetry with on_packet_sent (slow-start-only) is safe for the common paths but worth documenting more explicitly (see inline comment).
• Test coverage is thorough — all Outcome variants are exercised, bin propagation/reset logic is verified at boundaries, and the end-to-end exit test with delivery rate degradation is convincing. The #[cfg(debug_assertions)] zero-RTT panic test is a reasonable compromise given the debug_assert! approach.

No blocking issues found. The inline comments are minor documentation suggestions.

Solid implementation of SEARCH draft-09. The integer-arithmetic approach is clean, the circular buffer sizing is correct (with tight but sufficient margins), and all spec divergences are documented with reasoning and links.

What looks good:

• The cumulative byte counter design (separate record_acked_bytes + on_packets_acked) correctly handles the app-limited case where only the counter needs updating.
• Edge cases are well-guarded: zero bin_duration, RTT inflation beyond EXTRA_BINS, zero sent bytes, and warm-up period.
• The reset() on too-many-skipped-bins is a sensible optional extension of the spec, especially for app-limited senders.
• Test suite covers all Outcome variants and key state transitions (initialization, bin advancement, skipped bins, reset/reinit, delivery rate detection).

Architectural observation:
The SlowStart trait now has two SEARCH-specific methods with default no-op impls (record_acked_bytes, the sent_bytes parameter on on_packet_sent). This is a reasonable trade-off for now with three implementations, but if more slow-start algorithms are added, consider whether a richer event interface (e.g., a packet-event struct) would be cleaner than growing the trait signature.

No blocking issues found. The inline comments are minor suggestions for maintainability.